Skip to content

Modernize benchmark from-file - #927

Merged
mergify[bot] merged 4 commits into
vllm-project:mainfrom
dbutenhof:fix/fromfile
Jul 15, 2026
Merged

Modernize benchmark from-file#927
mergify[bot] merged 4 commits into
vllm-project:mainfrom
dbutenhof:fix/fromfile

Conversation

@dbutenhof

@dbutenhof dbutenhof commented Jul 13, 2026 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Collaborator

Summary

Modernize guidellm benchmark from-file by adopting the full registry typed --output style.

Details

The guidellm benchmark from-file command in 0.7.0

  1. Fails with --output-formats console due to an attempt to inject a path
  2. Does not allow specifying file paths for each format
  3. Lacks CLI style consistency with guidellm run

Replace the old --output-formats with the modern --output kind=<kind>,... style.

Test Plan

Manual command testing + tox

Related Issues

  • "I certify that all code in this PR is my own, except as noted below."

Use of AI

  • Includes code generated or substantially modified by an AI agent
  • Includes tests generated or substantially modified by an AI agent

NOTE: the Generated-by or Assisted-by trailers should be used in git commit messages when code or tests were generated or substantially modified by an AI agent, as described in the project's DEVELOPING.md file.

git log

commit 1ef3077
Author: David Butenhof dbutenho@redhat.com
Date: Mon Jul 13 10:03:20 2026 -0400

Modernize benchmark from-file

Signed-off-by: David Butenhof <dbutenho@redhat.com>

commit e35f0a8
Author: David Butenhof dbutenho@redhat.com
Date: Mon Jul 13 12:33:42 2026 -0400

Migration update

Signed-off-by: David Butenhof <dbutenho@redhat.com>

commit 49128ac
Author: David Butenhof dbutenho@redhat.com
Date: Tue Jul 14 15:32:14 2026 -0400

Fix benchmark from-file tests

Replace the disabled re-export tests with working code.

Signed-off-by: David Butenhof <dbutenho@redhat.com>

commit bfd6e5d
Author: David Butenhof dbutenho@redhat.com
Date: Wed Jul 15 14:48:14 2026 -0400

Adjust unit tests

Signed-off-by: David Butenhof <dbutenho@redhat.com>

Signed-off-by: David Butenhof dbutenho@redhat.com

@dbutenhof dbutenhof self-assigned this Jul 13, 2026
@dbutenhof dbutenhof added feature Represents a new user-visible feature bug Represents a user-visible defect escape A problem found by an external user in a GuideLLM release. cli Work related to the GuideLLM CLI labels Jul 13, 2026
jaredoconnell
jaredoconnell previously approved these changes Jul 13, 2026
View reviewed changes

@jaredoconnell jaredoconnell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works well. One thing to consider is that it overwrites the old benchmark, which is fine, but if there is ever a bug in it then it could corrupt the results file.

@mergify

mergify Bot commented Jul 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Queued — the merge queue status continues in this comment ↓.

@dbutenhof

dbutenhof commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator Author

It works well. One thing to consider is that it overwrites the old benchmark, which is fine, but if there is ever a bug in it then it could corrupt the results file.

Certainly true -- but not new. I had a couple of ideas along the way that I decided not to pursue, but I'm not sure any of it makes much sense. The user controls the output file, and if they don't want to overwrite the input on kind=json they can easily supply a distinct pathname for that output.

@dbutenhof
dbutenhof requested a review from SkiHatDuckie July 13, 2026 20:13
@dbutenhof dbutenhof mentioned this pull request Jul 14, 2026
Closed
4 tasks
SkiHatDuckie
SkiHatDuckie approved these changes Jul 14, 2026
View reviewed changes

@SkiHatDuckie SkiHatDuckie left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me as well. I wonder if it would be a good idea to fix the tests in test_benchmark_from_file_entrypoint.py at some point, so that manual testing isn't as necessary.

@dbutenhof

dbutenhof commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

Works for me as well. I wonder if it would be a good idea to fix the tests in test_benchmark_from_file_entrypoint.py at some point, so that manual testing isn't as necessary.

I hadn't actually noticed that there are no tests, because they're all disabled. I'm both tempted to fix that and also tempted to just merge the fix and worry about that later...

jaredoconnell
jaredoconnell reviewed Jul 14, 2026
View reviewed changes
Comment thread pyproject.toml
@dbutenhof
dbutenhof requested a review from jaredoconnell July 14, 2026 20:29
jaredoconnell
jaredoconnell previously approved these changes Jul 14, 2026
View reviewed changes

@jaredoconnell jaredoconnell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love the use of a new test dependency, but it's a test dependency. It seems okay.

@dbutenhof

dbutenhof commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

I don't love the use of a new test dependency, but it's a test dependency. It seems okay.

What I find interesting is that we're doing similar things in product code. Possibly not directly in async methods, but I found several in blocking methods called by async methods, and since we don't seem to be disabling ASYNC240 its not clear why we haven't seen complaints about those usages. Maybe ruff just isn't smart enough to find them?

SkiHatDuckie
SkiHatDuckie approved these changes Jul 15, 2026
View reviewed changes

@SkiHatDuckie SkiHatDuckie left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also unsure why ruff needs the new dependency for this file specifically, but everything seems to be working.

@jaredoconnell jaredoconnell mentioned this pull request Jul 15, 2026
Merged
3 tasks
dbutenhof added 4 commits July 15, 2026 14:02
@dbutenhof
Modernize benchmark from-file
1ef3077 
Signed-off-by: David Butenhof <dbutenho@redhat.com>
@dbutenhof
Migration update
e35f0a8 
Signed-off-by: David Butenhof <dbutenho@redhat.com>
@dbutenhof
Fix benchmark from-file tests
49128ac 
Replace the disabled re-export tests with working code.

Signed-off-by: David Butenhof <dbutenho@redhat.com>
@dbutenhof
Adjust unit tests
bfd6e5d 
Signed-off-by: David Butenhof <dbutenho@redhat.com>
@dbutenhof

dbutenhof commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

@Mergifyio queue

@mergify

mergify Bot commented Jul 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Merge Queue Status

This pull request spent 6 minutes 31 seconds in the queue, including 6 minutes 11 seconds running CI.

Required conditions to merge
  • github-review-approved [🛡 GitHub repository ruleset rule Merge Requirements]
  • any of [🛡 GitHub repository ruleset rule Merge Requirements]:
    • check-success = @github-actions/quality (3.10) / type-checks
    • check-neutral = @github-actions/quality (3.10) / type-checks
    • check-skipped = @github-actions/quality (3.10) / type-checks
  • any of [🛡 GitHub repository ruleset rule Merge Requirements]:
    • check-success = @github-actions/quality (3.10) / precommit-checks
    • check-neutral = @github-actions/quality (3.10) / precommit-checks
    • check-skipped = @github-actions/quality (3.10) / precommit-checks
  • any of [🛡 GitHub repository ruleset rule Merge Requirements]:
    • check-success = @github-actions/quality (3.10) / quality-checks
    • check-neutral = @github-actions/quality (3.10) / quality-checks
    • check-skipped = @github-actions/quality (3.10) / quality-checks
  • any of [🛡 GitHub repository ruleset rule Merge Requirements]:
    • check-success = @github-actions/tests (3.10) / e2e-tests
    • check-neutral = @github-actions/tests (3.10) / e2e-tests
    • check-skipped = @github-actions/tests (3.10) / e2e-tests
  • any of [🛡 GitHub repository ruleset rule Merge Requirements]:
    • check-success = @github-actions/tests (3.10) / integration-tests
    • check-neutral = @github-actions/tests (3.10) / integration-tests
    • check-skipped = @github-actions/tests (3.10) / integration-tests
  • any of [🛡 GitHub repository ruleset rule Merge Requirements]:
    • check-success = @github-actions/tests (3.10) / unit-tests
    • check-neutral = @github-actions/tests (3.10) / unit-tests
    • check-skipped = @github-actions/tests (3.10) / unit-tests
  • any of [🛡 GitHub repository ruleset rule Merge Requirements]:
    • check-success = @github-actions/update-description
    • check-neutral = @github-actions/update-description
    • check-skipped = @github-actions/update-description

@mergify mergify Bot added the queued label Jul 15, 2026
mergify Bot added a commit that referenced this pull request Jul 15, 2026
@mergify
Merge of #927
97826e1
@mergify mergify Bot mentioned this pull request Jul 15, 2026
Closed
35 tasks
@mergify
mergify Bot merged commit 90bc9de into vllm-project:main Jul 15, 2026
12 checks passed
@mergify mergify Bot removed the queued label Jul 15, 2026
@bennyturns bennyturns mentioned this pull request Jul 19, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaredoconnell jaredoconnell jaredoconnell approved these changes
@sjmonson sjmonson Awaiting requested review from sjmonson
+1 more reviewer
@SkiHatDuckie SkiHatDuckie SkiHatDuckie approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@dbutenhof dbutenhof

Labels

bug Represents a user-visible defect cli Work related to the GuideLLM CLI escape A problem found by an external user in a GuideLLM release. feature Represents a new user-visible feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

guidellm benchmark from-file crashes with ValidationError when output includes console format

3 participants

@dbutenhof @jaredoconnell @SkiHatDuckie